Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dangerfile and provide bundle size info #2608

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

mAAdhaTTah
Copy link
Member

@mAAdhaTTah mAAdhaTTah commented Oct 25, 2020

Fixes #1787.

@github-actions
Copy link

github-actions bot commented Oct 25, 2020

No JS Changes

Generated by 🚫 dangerJS against 7b921a7

@mAAdhaTTah mAAdhaTTah force-pushed the add-bundlesize-bot branch 5 times, most recently from cb9150b to 1037fa6 Compare October 25, 2020 12:37
@mAAdhaTTah
Copy link
Member Author

This is fairly rudimentary but basically does the thing. I can add additional comparisons to the table, if desired, adding comparisons to the source files and/or adding comparisons to non-gzipped versions.

The reason I didn't use a lib like bundlesize is they enforce a max per file or glob checked and I don't think the purposes here is to enforce a max so much as provide additional information for evaluating adding few features.

Open to comments/suggestions, but this is ready for review!

@mAAdhaTTah mAAdhaTTah marked this pull request as ready for review October 25, 2020 12:40
@mAAdhaTTah mAAdhaTTah changed the title Test adding Dangerfile and running in action Add Dangerfile and provide bundle size info Oct 25, 2020
@mAAdhaTTah mAAdhaTTah force-pushed the add-bundlesize-bot branch 3 times, most recently from 51c6697 to 747ee99 Compare October 25, 2020 14:22
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good but there are still some things to improve.

Could you please add a summary and the absolute change? It's nice that know the relative change for each file, but the absolute changes (per file and in total) are also interesting.

dangerfile.js Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

Hmmm. Did the file size change? I'm pretty sure prism-core.min.js was at 2.93KB (both master and pull) when I first saw the message.

@mAAdhaTTah
Copy link
Member Author

@RunDevelopment Yes, we went from 1024 -> 1000 for the calculation to make KB accurate.

@RunDevelopment
Copy link
Member

Ahh, yeah. I'm stupid. Maybe I could also remember what I say and agree to. That'd be great.

@mAAdhaTTah
Copy link
Member Author

lol don't be so hard yourself.

@mAAdhaTTah
Copy link
Member Author

mAAdhaTTah commented Oct 25, 2020

Ok, so need to do some testing w/ deleted files & remote forks, but before I dive into that, how does the output look? I narrowed it down to JS files in the plugins & components folders. Do we want CSS files too?

dangerfile.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

CSS files rarely change, so I don't think that's necessary. Unminified files shouldn't be included either (otherwise I will remove all comments to keep those file sizes down 😄).
Your initial approach (all .min.js files) seemed fine to me.

Regarding the output, I think it's fine.
A summary would be nice if more than 1 minified file changes. Though it might not be a good idea to just add up the gzipped differences. GZip will likely optimize between files in a real bundle (I'm thinking one file with all languages). I don't know what the best solution for this is.

@mAAdhaTTah
Copy link
Member Author

A summary would be nice if more than 1 minified file changes.

Do you have something in mind for this? What sort of details would you want in a summary?

Though it might not be a good idea to just add up the gzipped differences. GZip will likely optimize between files in a real bundle (I'm thinking one file with all languages). I don't know what the best solution for this is.

Yeah, that makes sense. The goal isn't strictly to provide exact figure for all these use cases but to give us info on the tradeoffs we're making. Relatedly, when we get the benchmark tests in (I'm going to take a run through the open PRs over the coming week), we can add those here for some additional info.

@RunDevelopment
Copy link
Member

A summary would be nice if more than 1 minified file changes.

Do you have something in mind for this? What sort of details would you want in a summary?

Nothing grand. Just the number of relevant files changed and the total number of bytes saved/gained.

The goal isn't strictly to provide exact figure for all these use cases but to give us info on the tradeoffs we're making.

In that case, we might as well just add up the gzipped differences for now and replace it with something better later.

Relatedly, when we get the benchmark tests in [...], we can add those here for some additional info.

Yeah no. The benchmark in its current form takes about 2 to 5 minutes (depends on the configuration) to run after all files have been downloaded. I don't want us to wait that long.

But we could make the benchmark start via a manual trigger. Idk, make a 🚀 reaction to the comment or something.


Something I hope to implement with this is what I said in #1758. If the PR changes the language definitions, the bot will post a link to the test page, so you can just try out the changes.

I also really like the example with test files here since we have the same situation. I'll probably make a few PRs after this one is merged.

@mAAdhaTTah
Copy link
Member Author

Summary is added.

@mAAdhaTTah mAAdhaTTah force-pushed the add-bundlesize-bot branch 5 times, most recently from 423ac9f to 1e1a32b Compare November 8, 2020 20:19
@mAAdhaTTah
Copy link
Member Author

mAAdhaTTah commented Nov 8, 2020

Ok, I think we're looking pretty good but let me know what you think! I'll fix up the failures once you sign off on the results.

dangerfile.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

I'm pretty sure the renaming-solution you implemented won't work.
Git's renaming detection is pretty good. It will even detect renames in files that marginally changed. This means that we can't assume that the content of the original and renaming file are the same. This is relevant because the diff in the summary will be wrong if we make this assumption.

In your test case, git wasn't able to detect the rename because you tested with minified files. Git's diff algorithm is line-based, so if you rename + change a one-line file, git won't be able to detect the rename. Please rename a file without changing it. Your current test case is just a deleted and an added file as far as git is concerned.

The problem with renames is that we don't know the name of the original file when using Danger. Danger only adds the renamed file in modified_files. You can't get the original file name. In #2616, I used git's API directly just so that I could disable git's renaming detection.

@mAAdhaTTah
Copy link
Member Author

mAAdhaTTah commented Nov 8, 2020

You're right in the abstract, but I was going for a best-guess solution that will produce a somewhat-valid output instead of erroring. I don't think we're going to end up renaming minified files that often, so I don't think we need to maintain a bunch of logic to make sure it's perfect. We could display a warning saying "hey whoops this was renamed, we don't know" but I think it'll be pretty clear that it'll look kinda like a new file but the diff is a rename.

Your current test case is just a deleted and an added file as far as git is concerned.

So basically, yes, and I think that's ok.

@RunDevelopment
Copy link
Member

RunDevelopment commented Nov 8, 2020

maintain a bunch of logic

So you agree to use the correct solution ;)

I mean, using git directly sounds scary but it's just 2 lines to get all changed files without renames.

const result = await git.diff(['--name-only', '--no-renames', 'master...']);
return result ? result.split(/\r?\n/g) : []; // return an array of changed files

These two lines mean that we don't have to worry about renames all. Renamed files are just one deleted file and one added file. Getting the file content is simple:

const [fileContents, fileMasterContents] = await Promise.all([
	fs.readFile(file, 'utf-8').catch(() => null),
	git.show([`master:${file}`]).catch(() => null),
]);

That's a lot simpler than the current error-handling solution, isn't it?

@mAAdhaTTah
Copy link
Member Author

Fair enough. Implemented that. Kept it catch(() => '') and added silent to simplegit cuz the git show was erroring to the console.

@mAAdhaTTah mAAdhaTTah force-pushed the add-bundlesize-bot branch 2 times, most recently from ae966eb to e483b26 Compare November 9, 2020 00:33
@mAAdhaTTah
Copy link
Member Author

Alright, I think that gets us there.

dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor nits and then I think we're done.

Also, please add dangerfile.js to .npmignore. (Or maybe we could add a files section to package.json and ditch .npmignore instead? Might be easier looking forward.)

@mAAdhaTTah
Copy link
Member Author

Ok, I've added your last suggestions, removed the failures, and squashed everything down. Are we good to go?

@RunDevelopment RunDevelopment merged commit 9df20c5 into master Nov 9, 2020
@RunDevelopment RunDevelopment deleted the add-bundlesize-bot branch November 9, 2020 14:36
@RunDevelopment
Copy link
Member

@mAAdhaTTah This is something I wanted to ask: Why do you always squash the commit? I always "squash and merge" on GitHub anyway.

@mAAdhaTTah
Copy link
Member Author

@RunDevelopment By default, when GitHub squash & merges, the commit message is going to be a list of all the commits squashed together, which I don't really like as a commit message. I could rewrite the message as I merge, but in the past, I've forgotten to do that, and since I'm thinking about the commit message when I'm making the commit, I prefer to combine them into the one commit, with the message that I want, while I'm committing.

In this case in particular, because there was stuff in the commits I wanted to remove, I actually rolled back all of the commits locally and readded the commit with just the real changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add buildsize bot
2 participants